Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add LMS icons #3029

Merged
merged 1 commit into from
Apr 25, 2024
Merged

feat: add LMS icons #3029

merged 1 commit into from
Apr 25, 2024

Conversation

brian-smith-tcril
Copy link
Contributor

@brian-smith-tcril brian-smith-tcril commented Apr 3, 2024 •
edited
Loading

Description

Resolves #3024

image

Deploy Preview

https://deploy-preview-3029--paragon-openedx.netlify.app/foundations/lms-icons/

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • Were your changes tested in the example app?
  • Is there adequate test coverage for your changes?
  • Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

  • Verify your changes were released to NPM at the expected version.
  • If you'd like, share your contribution in #show-and-tell.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@brian-smith-tcril brian-smith-tcril changed the title Lms icons feat: add LMS icons Apr 3, 2024
@netlify Netlify
Copy link

netlify bot commented Apr 3, 2024 •
edited
Loading

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 2f68e54
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/6629650c83856f0009a4ae5d
😎 Deploy Preview https://deploy-preview-3029--paragon-openedx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@brian-smith-tcril brian-smith-tcril marked this pull request as draft April 3, 2024 16:24
@brian-smith-tcril brian-smith-tcril marked this pull request as ready for review April 5, 2024 17:46
@codecov Codecov
Copy link

codecov bot commented Apr 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.18%. Comparing base (d67a04f) to head (48e51cf).
Report is 1 commits behind head on master.

❗ Current head 48e51cf differs from pull request most recent head 2f68e54. Consider uploading reports for the commit 2f68e54 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #3029   +/-   ##
=======================================
  Coverage   93.18%   93.18%           
=======================================
  Files         249      249           
  Lines        4342     4342           
  Branches     1036     1036           
=======================================
  Hits         4046     4046           
  Misses        292      292           
  Partials        4        4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brian-smith-tcril brian-smith-tcril mentioned this pull request Apr 10, 2024
Closed
10 tasks
ihor-romaniuk
ihor-romaniuk reviewed Apr 11, 2024
View reviewed changes
icons/svg/lms_book.svg
>
<path
d="M7.5 22C6.53333 22 5.70833 21.6583 5.025 20.975C4.34167 20.2917 4 19.4667 4 18.5V5.5C4 4.53333 4.34167 3.70833 5.025 3.025C5.70833 2.34167 6.53333 2 7.5 2H20V17C19.5833 17 19.2292 17.1458 18.9375 17.4375C18.6458 17.7292 18.5 18.0833 18.5 18.5C18.5 18.9167 18.6458 19.2708 18.9375 19.5625C19.2292 19.8542 19.5833 20 20 20V22H7.5ZM6 15.325C6.23333 15.2083 6.475 15.125 6.725 15.075C6.975 15.025 7.23333 15 7.5 15H8V4H7.5C7.08333 4 6.72917 4.14583 6.4375 4.4375C6.14583 4.72917 6 5.08333 6 5.5V15.325ZM10 15H18V4H10V15ZM7.5 20H16.825C16.725 19.7667 16.6458 19.5292 16.5875 19.2875C16.5292 19.0458 16.5 18.7833 16.5 18.5C16.5 18.2333 16.525 17.975 16.575 17.725C16.625 17.475 16.7083 17.2333 16.825 17H7.5C7.06667 17 6.70833 17.1458 6.425 17.4375C6.14167 17.7292 6 18.0833 6 18.5C6 18.9333 6.14167 19.2917 6.425 19.575C6.70833 19.8583 7.06667 20 7.5 20Z"
fill="#707070"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] Should we add SVG icons with a hard-code colors here and in other new icons?

But after generation SVG to JSX and ED5 icon inherit color as expected
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded colors in the svgs go away once the icons are generated

paragon/icons/svg/lms_book_complete.svg

Line 16 in 48e51cf

fill="#0D7D4D"

vs

paragon/icons/jsx/LmsBookComplete.jsx

Line 13 in 48e51cf

fill="currentColor"

so I wasn't concerned about the svgs having hardcoded colors. If we feel a need to remove those we can.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not a problem for me, I just decided to clarify)

icons/svg/lms_completion_solid.svg Outdated Show resolved Hide resolved
@brian-smith-tcril brian-smith-tcril force-pushed the lms-icons branch 2 times, most recently from 6e41659 to f1c2014 Compare April 11, 2024 13:51
@ihor-romaniuk
Copy link
Contributor

@brian-smith-tcril If we don't need any additional reviewers, it looks like ready for merge.

@brian-smith-tcril
Copy link
Contributor Author

Based on the conversation from today's working group meeting https://openedx.atlassian.net/wiki/spaces/BPL/pages/4179132431/2024-04-17+Meeting+notes I will update the icons to not have hardcoded white for the checkmarks in the "complete" icons

@brian-smith-tcril
Copy link
Contributor Author

After a bit of investigation I found the circle with the checkmark is simply covering up the rest of the icon, and relies quite heavily on the fill and stroke. I'm not very experienced with advanced clip masking in svg, if someone else is more experienced and willing to take a stab at it that would be wonderful, if not I can do a deep dive into it tomorrow.

@brian-smith-tcril @ihor-romaniuk
feat: add LMS icons
2f68e54 
Co-authored-by: ihor-romaniuk <[email protected]>
@brian-smith-tcril brian-smith-tcril merged commit 784f604 into openedx:master Apr 25, 2024
8 checks passed
@openedx-semantic-release-bot
Copy link

🎉 This PR is included in version 22.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

bradenmacdonald
bradenmacdonald reviewed Apr 25, 2024
View reviewed changes
icons/es5/LmsEditsquare.js
fill: "currentColor"
}));
};
export default SvgLmsEditsquare;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brian-smith-tcril In this PR, you committed some files with names that differ only in case:

  • icons/es5/LmsEditSquare.js andicons/es5/LmsEditsquare.js
  • icons/es5/LmsEditSquareComplete.js and icons/es5/LmsEditsquareComplete.js
  • same files in the jsx folder

This is causing all kinds of havoc on my Mac where the file system is case-insensitive and git keeps toggling back and forth between both versions of the file with a single name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3035

@brian-smith-tcril brian-smith-tcril mentioned this pull request Apr 25, 2024
Merged
10 tasks
@brian-smith-tcril brian-smith-tcril deleted the lms-icons branch April 29, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradenmacdonald bradenmacdonald bradenmacdonald left review comments

@ihor-romaniuk ihor-romaniuk ihor-romaniuk approved these changes

@adamstankiewicz adamstankiewicz adamstankiewicz approved these changes

@arbrandes arbrandes Awaiting requested review from arbrandes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add LMS icons
5 participants
@brian-smith-tcril @ihor-romaniuk @openedx-semantic-release-bot @bradenmacdonald @adamstankiewicz